-
Notifications
You must be signed in to change notification settings - Fork 3.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for interaction between import() and microtask queue #35949
Add tests for interaction between import() and microtask queue #35949
Conversation
1b880cc
to
3f80923
Compare
So I think we have two consistent options:
It sounds like Safari follows option (2), and no browser follows option (1). So a path of least resistance would be to spec (2) and update Chrome/Firefox. @hiroshige-g, what do you think from Chrome's perspective? |
For context, I discovered this behavior when working on tc39/ecma262#2905 / whatwg/html#8253. |
When we implemented module loading, I interpreted the "asynchronous completion" in the spec as something like posting a task to make it always async, while whatwg/html#8264 made it explicitly synchronous in some cases. What was the intention of the "asynchronous completion"? So the Chromium implementation is roughly: (3) Always schedule at least one full task (i.e., always drain the microtask queue) for the fetch * module graph algorithms This is consistent with the test result. import() with invalid URL is rejected synchronously because module specifier resolution is outside the fetch * module graph algorithms. |
Note that URL resolution/validation happens inside fetch an import() module script graph, and before whatwg/html#8264 HostImportModuleDynamically called that algorithm using the "asynchronous completions" pattern (so it should have been consistent with the other parts of fetch ... that do not involve network requests). |
I can't really say :(. I was bad and didn't think properly about it.
As @nicolo-ribaudo, I don't think that's correct. HostImportModuleDynamically calls fetch an import() module script graph, which if resolution fails, used the "asynchronously complete this algorithm with null" wording. After whatwg/html#8264, it uses sync wording, but we're wondering what the best path is... Re-reading the algorithms I noticed we have a number of early-failure modes, actually:
We should probably be consistent with all of these? (And ideally even test all of them.) So generalizing, I think the options on the table are:
Reiterating my earlier comment, Chrome's current behavior feels inconsistent to me so I would prefer (1) or (2). And Safari matches (2). |
I added a test for invalid EDIT: The Safari ❌ maybe be related to the fact that it doesn't properly validate assertions (https://wpt.fyi/results/html/semantics/scripting-1/the-script-element/import-assertions/invalid-type-assertion-error.html?label=master&label=experimental&aligned=&view=subtest&q=import-assertions) but (I think, based on the Webkit source code because I don't have access to Safari TP) just fallbacks to |
Thanks for this @nicolo-ribaudo. Option 2 makes the most sense to me, I don't think there will be an issue updating our behavior to match. I think our interpretation of the asynchronous completions was the similar to hiroshige's, but jonco can confirm if that is true. ccing @jonco3 and @smaug---- to get a couple more eyes from mozilla on this. I don't believe assertions have shipped yet on our side (https://bugzilla.mozilla.org/show_bug.cgi?id=1777526). |
Now that https://bugs.webkit.org/show_bug.cgi?id=246207 has been fixed, the Safari column should be all ✔️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look great modulo a minor fix, and they test the desired outcome.
It would be ideal if you also expanded to test:
- moduleType is "css" and we're inside a WorkerGlobalScope
- use inside WorkletGlobalScope
- use inside ServiceWorkerGlobalScope
but of course we're happy to accept just these too.
I believe the spec as written also aligns with these tests, correct? In which case we can merge this shortly.
To fix CI failures, often rebasing on master works.
html/semantics/scripting-1/the-script-element/module/dynamic-import/microtasks/basic.html
Outdated
Show resolved
Hide resolved
Is there a way to say "run this test in a worker/etc", or do I have to write a normal test that spins up a worker/etc?
Yes 👍 |
The easiest way is to use Worklets are harder. I guess maybe modify the test I added in 43fb3b2 . |
6382a5b
to
19e9d13
Compare
I'm not sure about what to do with the CI failure, it looks like the basic tests sometimes timeout in Firefox. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The CI is often flaky (see #21706) and so in these cases we summon one of the admins to help force-merge... I'll work on that.
...semantics/scripting-1/the-script-element/module/dynamic-import/microtasks/worklet.https.html
Outdated
Show resolved
Hide resolved
…mport/microtasks/worklet.https.html
The HTML algorithms for dynamic import (starting from HostImportModuleDynamically) only schedule a task a when actually fetching the corresponding files, and it never happens if the imported module (and all its dependencies) have already been loaded (when step 5 of fetch a single module script doesn't happen and step 6 returns early).
I didn't know how to test "it doesn't schedule a task", so the tests are written using a loop that schedules many microtasks and checking if all those microtasks are run or not before that the
import()
promise is resolved.Browsers implement this inconsistently. This PR adds three new tests, and some of them fail:
It's possible that the Firefox/Chrome behavior is because the HTML spec uses a rare "asynchronous completions" wording which isn't defined anywhere. However, "asynchronous completions" are used for all the test cases, so I would expect the 1st to always behave like the 2nd (since they both only involve asynchronous completions and no fetching). I also opened whatwg/html#8264 to move away from that wording.
Footnotes
The fix has not been released yet, so it fails on CI. ↩